Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PLUGIN-1826] Error Management HTTP Source and Sink and fix sonar issues #180

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Amit-CloudSufi
Copy link

@Amit-CloudSufi Amit-CloudSufi commented Dec 6, 2024

This comment was marked as resolved.

@psainics psainics added the build label Dec 9, 2024
@Amit-CloudSufi Amit-CloudSufi changed the title Added HTTP error detail provider and refactored HTTP-sink and source package to handle error provider and fix sonar issues [PLUGIN-1826] Error Management HTTP Source and Sink and fix sonar issues Dec 9, 2024
@Amit-CloudSufi Amit-CloudSufi force-pushed the httpPluginErrorMang branch 2 times, most recently from e612cd0 to d273336 Compare December 12, 2024 11:59
@psainics psainics added build and removed build labels Dec 18, 2024
…ckage to handle error provider, fix sonar issues and added Validation error for linear retry duration
Copy link
Member

@itsankit-google itsankit-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix unit tests

errorMessage, String.format("Error message: %s", errorMessage),
ErrorUtils.getActionErrorByStatusCode(httpStatusCode).getErrorType(),
true, ErrorCodeType.HTTP, String.valueOf(httpStatusCode),
"https://developer.mozilla.org/en-US/docs/Web/HTTP/Status", new IllegalStateException(errorMessage));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use official documentations link: https://datatracker.ietf.org/doc/html/rfc7231#section-6

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
nextPageUrl, httpStatusCode, response.getBody());
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't do redundant Error message: %s

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed from all files

@@ -122,8 +130,13 @@ protected BasePage getNextPage() throws IOException {
case SUCCESS:
break;
case STOP:
throw new IllegalStateException(String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
nextPageUrl, httpStatusCode, response.getBody()));
String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorReason

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

throw new IllegalArgumentException("Invalid URL: " + configURL, e);
} catch (IOException e) {
LOG.warn("Error making {} request to URL {}.", config.getMethod(), config.getUrl());
String errorMessage = "Unable to make request. ";
Copy link
Member

@itsankit-google itsankit-google Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not an actional error reason

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

LOG.warn("Error making {} request to URL {}.", config.getMethod(), config.getUrl());
String errorMessage = "Unable to make request. ";
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

if (!shouldRetry) {
messageBuffer.clear();
retryCount = 0;
String errorMessage = String.format("Unable to execute HTTP request to %s.", url);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorReason

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

retryCount = 0;
String errorMessage = String.format("Unable to execute HTTP request to %s.", url);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage), ErrorType.SYSTEM, true, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

String errorMessage = String.format("Unable to execute HTTP request to %s.", url);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage), ErrorType.SYSTEM, true, e);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

} catch (Exception e) {
String errorMessage = String.format("Unexpected error occurred while executing HTTP request to URL: %s", url);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -247,16 +263,16 @@ public CloseableHttpClient createHttpClient(String pageUriStr) throws IOExceptio
URI uri = URI.create(pageUriStr);
AuthScope authScope = new AuthScope(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()));
credentialsProvider.setCredentials(authScope,
new UsernamePasswordCredentials(config.getUsername(), config.getPassword()));
new UsernamePasswordCredentials(config.getUsername(), config.getPassword()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unintended changes.

Comment on lines +274 to +275
new UsernamePasswordCredentials(
config.getProxyUsername(), config.getProxyPassword()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unintended changes

}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -311,7 +324,7 @@ private Header getAuthorizationHeader(AccessToken accessToken) {
private List<PlaceholderBean> getPlaceholderListFromURL() {
List<PlaceholderBean> placeholderList = new ArrayList<>();
if (!(config.getMethod().equals(REQUEST_METHOD_PUT) || config.getMethod().equals(REQUEST_METHOD_PATCH) ||
config.getMethod().equals(REQUEST_METHOD_DELETE))) {
config.getMethod().equals(REQUEST_METHOD_DELETE))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unintended changes

Comment on lines +366 to +370
.await().with()
.pollInterval(pollInterval)
.pollDelay(config.getWaitTimeBetweenPages(), TimeUnit.MILLISECONDS)
.timeout(config.getMaxRetryDuration(), TimeUnit.SECONDS)
.until(this::executeHTTPServiceAndCheckStatusCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unintended changes

String errorMessage = "Error while executing http request for remaining input messages" +
" after the batch execution.";
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't we have status code information here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

} catch (Exception e) {
throw new RuntimeException("Error while executing http request for remaining input messages " +
"after the batch execution. " + e);
String errorMessage = "Error while executing http request for remaining input messages" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add http request information here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

errorMessage, String.format("Error message: %s", errorMessage),
ErrorUtils.getActionErrorByStatusCode(httpStatusCode).getErrorType(),
true, ErrorCodeType.HTTP, String.valueOf(httpStatusCode),
"https://developer.mozilla.org/en-US/docs/Web/HTTP/Status", new IllegalStateException(errorMessage));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here about documentation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
config.getUrl(), httpStatusCode, httpResponseBody);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, String.format("Error message: %s", errorMessage),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar comment here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

case SKIP:
case SEND:
LOG.warn(String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
config.getUrl(), httpStatusCode, httpResponseBody));
config.getUrl(), httpStatusCode, httpResponseBody));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert unintended changes

@@ -1,3 +1,4 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants